-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[chromecast] Make use of events #19319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
|
|
||
| <config-description uri="thing-type:chromecast:device"> | ||
| <parameter name="ipAddress" type="text" required="true"> | ||
| <parameter name="host" type="text" required="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the reviewer, the hostname can be used, so i think ipAddress is the wrong name, but it is also breaking, so i have some doubts to change it or not. I can also update the docs, that ipAddress can also contain a hostname, but meh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't it at least be explained in the parameter description? That wouldn't break anything.
| List<MediaStatus> statuses = event.getData(MediaStatusResponse.class).getStatuses(); | ||
| if (statuses != null && statuses.size() > 1) { | ||
| logger.warn("Received multiple media statuses, this is not supported. Statuses: {}", statuses); | ||
| } else if (statuses != null && !statuses.isEmpty()) { | ||
| statusUpdater.updateMediaStatus(statuses.getFirst()); | ||
| } else { | ||
| statusUpdater.updateMediaStatus(null); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| List<MediaStatus> statuses = event.getData(MediaStatusResponse.class).getStatuses(); | |
| if (statuses != null && statuses.size() > 1) { | |
| logger.warn("Received multiple media statuses, this is not supported. Statuses: {}", statuses); | |
| } else if (statuses != null && !statuses.isEmpty()) { | |
| statusUpdater.updateMediaStatus(statuses.getFirst()); | |
| } else { | |
| statusUpdater.updateMediaStatus(null); | |
| } | |
| MediaStatusResponse mediaStatusResponse = event.getData(MediaStatusResponse.class); | |
| List<MediaStatus> mediaStatuses = mediaStatusResponse == null ? null : mediaStatusResponse.getStatuses(); | |
| if (mediaStatuses == null) { | |
| statusUpdater.updateMediaStatus(null); | |
| } else { | |
| for (MediaStatus mediaStatus : mediaStatuses) { | |
| statusUpdater.updateMediaStatus(mediaStatus); | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be multiple statuses at times. There's no point in "rejecting" that, just apply them in order..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I remember, certain "actions" involve transitions that produce more than one media status in sequence.
| case APPEVENT: | ||
| logger.debug("Received an 'APPEVENT' event, ignoring"); | ||
| case RECEIVER_STATUS: | ||
| statusUpdater.processStatusUpdate(event.getData(ReceiverStatusResponse.class).getStatus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| statusUpdater.processStatusUpdate(event.getData(ReceiverStatusResponse.class).getStatus()); | |
| ReceiverStatusResponse receiverStatusResponse = event.getData(ReceiverStatusResponse.class); | |
| statusUpdater.processStatusUpdate(receiverStatusResponse == null ? null : receiverStatusResponse.getStatus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite what Eclipse's nullness logic claims, getData() very much can return null.
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
|
@lsiepel I guess this one drowned a bit among everything else. I'll try to look at it again. I got "sidetracked" a bit by starting to look at the Core mDNS implementation. It's far from ideal, but I don't quite know how to fix it. In a way, the way jmdns works is problematic and somewhat unclear. jmdns doesn't cache everything it sees, it only starts caching when it has a listener registered for a specific service. It's unclear to me if registering as a listener is enough for jmdns to actively probe (broadcast requests for devices providing that service to report back), or if you actually have to call The implementation for // Implementation note: The first time a list for a given type is
// requested, a ServiceCollector is created which collects service
// infos. This greatly speeds up the performance of subsequent calls
// to this method. The caveats are, that 1) the first call to this
// method for a given type is slow, and 2) we spawn a ServiceCollector
// instance for each service type which increases network traffic a
// little.A And, I'm still not sure exactly when an "active probe" for the service is being done. Ideally, you'd want jmdns to do an "active probe" for the Chromecast service when the binding starts, and then keep a Worse, OH's On top of that, OH spawns one jmdns client for each network interface, and then forwards "requests" to all of these. It's just complicated to know how to handle this efficiently and correctly, but it's clear to me that it's needed and that mDNS use in OH in general causes issues for users. I'm just not sure what the solution is. |
|
There are so many topics to work on, so this one was also not high on my list. I looked at your PR, merged it and now i have been fixing several other PR's and reviews i had lingering around. Your comment rather looks like a generic openHAB mDNS comment and not specific to chromecast. You say that mDNS usage leads to problems for many users. Do you have specific cases in mind? Maybe if we look after a specific case we can narrow it down, or make the problem smaller/reduce it. |
The comment is generic, but it's also tied to figuring out how to correctly handle mDNS here. As I've said before, I think one of the benefits of switching library is that it will no longer spin up its own jmdns instance that makes a mess over everything, because it's working "in parallel" with OH's jmdns instance, which means that "the OH computer" will make multiple, diverging responses to requests from other devices. With the current library, you don't have to think about mDNS, since it doesn't give you any choice, it will so what it wants to do. But this also causes problems. "Getting control" of mDNS also means that we now have to sort of how to handle it correctly.
There have been multiple forum threads about problems, where jmdns is high on the list of suspects. I've dug into thread dumps where there are loads of jmdns threads either deadlocked or "slow walking", waiting for other things, delaying "everything" and making OH slow or stop responding. It's hard to pinpoint exactly what leads up to these situations, when a thread dump is just a snapshot of what is going on at any given moment, but I suspect that things aren't handled efficiently and optimally, that things that should only be done once are done over and over again for example. mDNS is also "slow by nature" in that it's based on a model of broadcast -> reply, where the broadcast should be repeated 3 times (I think) with a given interval, and participants can respond basically "at any time", so you have to wait several seconds to be sure that you've captured all replies. Therefore, it's even more important that things aren't done more often than they should, with waiting times and all what follows. There have also been people that have reported "mDNS storms" on their networks when OH runs, and there are others that just want to disable it because it causes instability and slowness (hence the focus on disabling add-on finders). I'm not saying that OH is to blame for all this, mDNS is a "problematic standard" in itself, where it doesn't seem to me like the designers really though things through properly. So, there are all kinds of "ad hoc" solutions at work here, both in OSes and in devices. I've tried to find solutions to all this, but have mostly been banging my head against the wall. The mDNS standard is hard to read, because it keeps referencing the DNS standard, so you really need to know them both to get an overview. The jmdns code is written in a way that is very hard to "follow" for me, so I often just have to give up trying to get to the bottom - both because I don't fully know the standards (aka what it should be doing) and I can't figure out what it is doing. |
|
A lot is going on with mDNS, but we're normally hiding it from the log. Try setting I'm seeing a lot of double events from the Chromecast in the log, so either there's a "double event subscription" going on, or there's some problem with the library. edit: The audio sink is also registered twice according to the UI, so I'm assuming that the binding is a bit too eager. edit2: I find two of everything, channels, cast devices, input processing threads. So, I assume that the binding somehow creates two |
🤦 Or could it be that I had two Things configured for the same device? 😊 |
|
I'm trying to implement some additional events, and I'm looking at the multizone stuff. I've never taken the trouble to figure out what it is/how it's used. Do you have any idea? Is that something that would be useful for OH?
Again, I have no idea what this is, but I'm imagining that it might be relevant for OH it people use Chromecasts as audio sinks.. where maybe you can cast something to an entire group? |
Main purpose was to change the library to a more maintained version from @Nadahar.
This made it also possible to change from data polling to events, making the binding a bit more snappy with updates.
Improved scheduled task disposal.